Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use named arguments in DaskFinder.find_spec #90

Merged

Conversation

pavoljuhas
Copy link
Contributor

Use named arguments in DaskFinder.find_spec

Follow the base class argument names as in MetaPathFinder.find_spec per
https://docs.python.org/3.13/library/importlib.html#importlib.abc.MetaPathFinder.find_spec

This allows find_spec callers to use keyword arguments for path and target.

@pavoljuhas pavoljuhas requested a review from a team as a code owner February 28, 2025 19:14
Copy link

copy-pr-bot bot commented Feb 28, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@pavoljuhas
Copy link
Contributor Author

pavoljuhas commented Feb 28, 2025

This fixes the following import error for Cirq due to mismatched arguments of DaskFinder.find_spec

# in fresh venv for Python 3.12
$ pip install cirq-core rapids-dask-dependency
$ python -c "import cirq"
...
  File "/tmp/t312/lib/python3.12/site-packages/cirq/_import.py", line 66, in find_spec
    spec = self.finder.find_spec(fullname, path=path, target=target)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: DaskFinder.find_spec() got an unexpected keyword argument 'path'

$ pip uninstall rapids-dask-dependency
$ python -c "import cirq"
# works as expected

xref: googlecolab/colabtools#5141
cc: @raydouglass

@pavoljuhas pavoljuhas changed the base branch from branch-25.04 to main February 28, 2025 19:27
@vyasr vyasr added bug Something isn't working non-breaking Introduces a non-breaking change labels Feb 28, 2025
@vyasr
Copy link
Contributor

vyasr commented Feb 28, 2025

Thanks for this fix @pavoljuhas! Do you know what version of RAPIDS is installed on the systems where this issue is being experienced? That changes the branch that we would merge this change into.

@pavoljuhas
Copy link
Contributor Author

pavoljuhas commented Feb 28, 2025

Do you know what version of RAPIDS is installed on the systems where this issue is being experienced?

The Colab environment where it shows has rapids_dask_dependency version 24.12.0.
The commit 2304660 which introduced the code is in all branches starting from branch-24.06, so ideally the fix would apply to all branches that are still maintained.

Thank you for quick response!

Copy link

@wence- wence- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes looks good to me. Also not sure exactly which branch this should go to

@rjzamora rjzamora changed the base branch from main to branch-25.04 March 3, 2025 14:36
@rjzamora
Copy link
Member

rjzamora commented Mar 3, 2025

Thank you for contributing this fix @pavoljuhas !

The only branch under development right now is branch-25.04 - We can definitely merge this into 25.04, but it is very unlikely that we can put out a patch release for 25.02 or earlier. Since that process requires significant time and resources, it's unlikely to happen unless the problem affects a large user base (or is related to security).

@pavoljuhas
Copy link
Contributor Author

We can definitely merge this into 25.04, but it is very unlikely that we can put out a patch release for 25.02 or earlier.

That is fine, I will ask colab maintainers to update the default colab environment once the next patch release of 25.04 is out.
Thank you for quick response!

PS: Is there anything else to do on my part to get this merged?

@rjzamora
Copy link
Member

rjzamora commented Mar 3, 2025

/ok to test

@rjzamora
Copy link
Member

rjzamora commented Mar 3, 2025

/merge

@rapids-bot rapids-bot bot merged commit 9cc96b5 into rapidsai:branch-25.04 Mar 3, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants